🔒 fix(security): remediate findings #740–#743 - #744
Merged
Conversation
- #740 path traversal: sanitize server-supplied attachment filename in `articles attach download` via Path(filename).name, confining writes to the working directory (falls back to attachment_<id> for empty/./.. names). - #741 cleartext transport: warn on stderr when a base URL uses http:// before the token is sent (warn-only, still proceeds) via warn_if_insecure_url(). - #742 shell hardening: tutorial executor now uses create_subprocess_exec with a parsed argv list instead of shell=True; removed the dead shell=True _execute_command path in tutorial/core.py. - #743: add SECURITY.md vulnerability disclosure policy. Incidental (to keep local pre-commit == CI, both pre-existing, not security): - align the pre-commit `ty` hook with the tox type gate (same 0.0.1a14 pin and ignore list) so it stops flagging unrelated warnings the CI gate ignores. - fix pre-existing pydocstyle nits on utils.loads_lenient docstring. Regression tests for #740/#741/#742 in tests/test_security_fixes.py; removed obsolete tests for the deleted _execute_command. Threat model: single-user local install. Built via Spec Kit; artifacts under specs/003-security-fixes/. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0166iS8CRLfRiyqCBaYtRDaR
|
Profile summary: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Remediates four security findings via the full Spec Kit workflow (artifacts in
specs/003-security-fixes/). Threat model: single-user local install — remote/network threats in scope.Closes #740. Closes #741. Closes #742. Closes #743.
Fixes
articles attach downloadsanitizes the server-supplied filename withPath(filename).name, confining writes to the working directory (falls back toattachment_<id>for empty/./..). Explicit--output(user's own path) is untouched.warn_if_insecure_url()prints a stderr warning when a base URL useshttp://, before the token is sent. Warn-only (still proceeds), per maintainer decision — keepshttp://localhost/on-prem working.create_subprocess_execwith a parsed argv list instead ofshell=True; shell metacharacters are inert. Removed the deadshell=True_execute_commandintutorial/core.py.SECURITY.md.Tests
tests/test_security_fixes.py— one regression test per behavioral fix (#740 traversal incl. absolute-path + normal-name cases; #741 http warns / https doesn't; #742 metacharacter payload creates no sentinel file). Removed obsolete tests for the deleted_execute_command. Full suite: 1399 passed.Incidental (pre-existing, not security)
Kept minimal but needed to land a clean, non-bypassed pre-commit run after the local toolchain drifted:
tyhook with the tox type gate (same0.0.1a14pin + ignore list) so it stops flagging pre-existing warnings in unrelated files that the CI gate already ignores.utils.loads_lenient's docstring (docstring-only, no behavior change).Verification
CI merge gates all pass locally:
ruff check/format,ty(a14), fullpytest, package build,zizmor. No--no-verify.🤖 Generated with Claude Code